Skip to content

HIVE-29634: Fix semijoin RHS column registration for multiple columns from same alias#6516

Open
tanishq-chugh wants to merge 3 commits into
apache:masterfrom
tanishq-chugh:HIVE-29634
Open

HIVE-29634: Fix semijoin RHS column registration for multiple columns from same alias#6516
tanishq-chugh wants to merge 3 commits into
apache:masterfrom
tanishq-chugh:HIVE-29634

Conversation

@tanishq-chugh
Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

Fix semijoin join-condition parsing so every RHS column from the same alias is registered

Why are the changes needed?

To fix the query with LEFT OUTER JOIN and CONCAT running into SemanticException

Does this PR introduce any user-facing change?

Yes, fixes a query running into SemanticException at runtime.

How was this patch tested?

Manual Testing + added a Qtest

if (rightAliases.size() > rhssize) { // the new table is rhs table
rhsAlias = rightAliases.get(rightAliases.size() - 1);
} else if (condn.getChild(0).getType() == HiveParser.TOK_TABLE_OR_COL) {
String alias = unescapeIdentifier(condn.getChild(0).getChild(0).getText().toLowerCase());
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need a comment here to explain what this else-if branch is doing, and why we couldn't simply rely on the recursive call to parseJoinCondPopulateAlias.

Also, do you think we need to check for nulls anywhere in condn.getChild(0).getChild(0).getText()? I am not sure if anything can be null here, could you please check?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @soumyakanti3578, Thanks for checking this!
Post your comment, i was going through the fix again and have found a better way to fix the issue where we are using the existing recursive call itself, this way the hardcoded code to fetch AST child written previously is avoided. Please check it out and let me know your thoughts. Thanks!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new approach is definitely better, thanks!

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

@soumyakanti3578 soumyakanti3578 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants